Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LiveReload functionality to Jekyll. #5142

Merged
merged 2 commits into from Dec 6, 2017
Merged

Conversation

awood
Copy link
Contributor

@awood awood commented Jul 27, 2016

Invoke using jekyll serve --livereload. The Serve command will inject
some necessary JavaScript links into pages. When loaded in a browser,
this JavaScript opens a connection over port 35729 using WebSockets to
an EventMachine reactor listening to that port in a separate thread.

When Jekyll finishes rendering a page, --livereload hooks create a
JSON message with the relative URL of the refreshed page. This message
is then pushed to the browser over the WebSockets connection. The
client-side JavaScript then reloads the page if the URL in the message
matches the URL of the current page.

Implements #4644

@awood
Copy link
Contributor Author

awood commented Jul 27, 2016

This PR doesn't include any significant documentation on the feature, but I figured I'd wait until some of the details get hammered out before writing anything.

Other notes:

  • It may be desirable to prune the number of options available to configure LiveReload. Specifically, the Flash version for browsers that don't support WebSockets might be worth dropping since no one is going to be developing there Jekyll site in Android and the overlap of IE 8 only users and Jekyll users is probably pretty small.
  • The semantics around --ignore are weird could probably lead to false bug reports. From my experiments and reading of the JavaScript (which I did not write), if any portion of URL the browser is pointed to matches the URL to reload in the JSON message, the browser will refresh. This is especially noticeable on index pages ("/blog/" would match every refresh message for pages in the directory)
  • The tests are very much functional tests instead of unit tests. In my opinion the functionality is complex enough to merit it, but others may disagree.

"Skips the initial site build which occurs before the server is started."],
"livereload" => ["-l", "--livereload",
"Use LiveReload to automatically refresh browsers"],
"swf" => ["--swf", "Use Flash for WebSockets support"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a no-go. Everybody is dropping support for flash faster than we want to turn around and have to remove it.

@awood
Copy link
Contributor Author

awood commented Jul 27, 2016

@envygeeks do you want revisions as separate commits to squash merge or for me to just adjust this current commit?

@envygeeks
Copy link
Contributor

@awood whatever works best for you and allows you to flow the best! We normally only ask for a rebase just before we merge it, before that, it's whatever workflow you wish to use since Github now has that fancy "view all files changed".

@envygeeks
Copy link
Contributor

I kinda like this, maybe we should add an updater like we've for normalize.css so that we can auto-update it every once in a while as it'll be quite easy to forget it exists. Other than that just a few comments and we can do another deeper review.

@awood
Copy link
Contributor Author

awood commented Jul 27, 2016

I kinda like this, maybe we should add an updater like we've for normalize.css so that we can auto-update it every once in a while as it'll be quite easy to forget it exists. Other than that just a few comments and we can do another deeper review.

So you automatically pull the file every so often? That's a really good idea. I think I will need to modify the code somewhat because it's been modified slightly to read from variables defined in the inserted script tags, but I think I can figure something else out so the stock livereload.js will work

@awood
Copy link
Contributor Author

awood commented Jul 28, 2016

So you automatically pull the file every so often? That's a really good idea. I think I will need to modify the code somewhat because it's been modified slightly to read from variables defined in the inserted script tags, but I think I can figure something else out so the stock livereload.js will work

Okay, I've implemented the above. The PR now just uses the stock version of the most recent released livereload.js. Pulling it in via bower might be a good way of making sure it stays up to date, but that is something for the maintainers to decide.

@awood awood force-pushed the livereload branch 2 times, most recently from cbfb322 to 80b4757 Compare July 29, 2016 20:15
@awood
Copy link
Contributor Author

awood commented Aug 1, 2016

Looks like this is failing in JRuby. I think the issue has to do with the WebSockets server thread introduced in this PR. I'll do some investigation, but if anyone out there has some experience with the subtleties of JRuby, I'd wouldn't mind a second opinion.

@awood awood force-pushed the livereload branch 2 times, most recently from 52f88a7 to 0382b3a Compare August 2, 2016 21:02
@awood
Copy link
Contributor Author

awood commented Aug 2, 2016

So I've made some changes for JRuby, but JRuby's TLS support is broken. I can either remove the TLS support for the WebSockets connection entirely or just ignore those options when running under JRuby (not sure if that's an acceptable solution though).

Additionally, while the tests pass, the actual functionality under JRuby is broken. For whatever reason, the message to reload is sent but the browser doesn't react. I'll look into this issue later and try and get some tests that actually ensure receipt of the reload command.

@awood
Copy link
Contributor Author

awood commented Sep 20, 2016

I have tracked the JRuby functionality issue down to a problem within either EM-Websocket or EventMachine. I've filed an issue with EM-Websocket.

@awood
Copy link
Contributor Author

awood commented Sep 20, 2016

@envygeeks the Travis failures for this PR appear to be code style releated in code that I haven't modified. Otherwise I think this PR is ready for review at least. There's an outstanding issue with EM-Websocket/EventMachine under JRuby but I don't think that should block the review process. I can go ahead and address any issues the maintenance team sees while waiting to hear back from on the issue I filed for the JRuby bug.

@envygeeks
Copy link
Contributor

@awood I'll take some time this week to start reviewing /cc @jekyll/core @jekyll/ecosystem

@envygeeks envygeeks assigned envygeeks and unassigned parkr Sep 20, 2016
@awood
Copy link
Contributor Author

awood commented Sep 20, 2016

Looks like the JRuby issue is a known problem with an EventMachine reactor running under a thread.

@awood
Copy link
Contributor Author

awood commented Sep 21, 2016

An alternative to having LiveReload just not work under JRuby would be to switch from WEBrick to an EventMachine compatible http server. Jekyll could then just start the EventMachine reactor when it's time to start serving and the reactor would manage both the LiveReload server on 35729 and the http server on 4000.

I actually have a working proof-of-concept using Thin as the http server, and it's pretty nice since it removes all the need for threading from Jekyll proper and just delegates everything to the EventMachine event loop. Plus EventMachine can daemonize so LiveReload would continue to work in daemon mode (whereas it does not in this PR since that would require Jekyll spinning off two processes which I thought a little dodgy especially if one process dies and the other doesn't).

The downside is that Thin doesn't run under JRuby at all. There are JRuby compatible http server (notably Puma) but they aren't EventMachine based and so there would have to be some sort of low-level shim layer that brokered between EventMachine's Connection class and the http server which would probably end up being a nightmare.

But if Thin ever runs under JRuby, I think the strategy described above would be a worthwhile refactoring.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good, just a few things that probably need changes. 😄

validate_options(opts)

opts["watch"] = true unless opts.key?("watch")
opts["livereload_port"] ||= LIVERELOAD_PORT
Copy link

@ghost ghost Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've used Hash#key? for "watch" and ||= for "livereload_port"

Copy link
Contributor Author

@awood awood Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I did that because "watch" can legitimately be false. I can line them up so they both use key. I think I tried to keep the ||= where possible because that seemed to be the preferred idiom elsewhere in the code.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, my mistake! 😊

" of LiveReload."
end
elsif opts["livereload_min_delay"] ||
opts["liverealod_max_delay"] ||
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo!

-              opts["liverealod_max_delay"]   ||
+              opts["livereload_max_delay"]   ||

opts["livereload_ignore"] ||
opts["livereload_port"]
Jekyll.logger.warn "The --livereload-min-delay, --livereload-max-delay, "\
"--livereoload-ignore, and --livereload-port options require the "\
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo!

-               "--livereoload-ignore, and --livereload-port options require the "\
+               "--livereload-ignore, and --livereload-port options require the "\

def start_callback(detached)
unless detached
proc do
Jekyll.logger.info("Server running...", "press ctrl-c to stop.")
@running << "."
Copy link

@ghost ghost Sep 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that Queue is the wrong data type for this usage. Correct me if I'm wrong, but you never seem to actually wait for the Queue to become populated and even if you did, there are probably better data types for this usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm using it as a mechanism to block the test from running until the server(s) can successfully start. The only place it's actually waiting for the queue to be populated is in test_commands_serve.rb. The test starts a Serve thread and Serve starts a WEBrick thread and joins it so there's not another good way I could think of to know whether WEBrick is actually up and running.

I think Queue is the appropriate choice here; the docs say "[The Queue] class provides a way to synchronize communication between threads."

That being said, I do think that it's kind of crummy to have cruft in there that's only really used for the tests. I'm totally open to suggestions on a better way to handle this situation.

@ghost
Copy link

ghost commented Sep 21, 2016

@awood

An alternative to having LiveReload just not work under JRuby would be to switch from WEBrick to an EventMachine compatible http server.
The downside is that Thin doesn't run under JRuby at all.

😆

I've never liked LiveReload but I'm sure it'll be a very useful feature for many Jekyll users.

@envygeeks
Copy link
Contributor

I've never really found it useful, I can F5 as fast as that thing loads lol. But at this point we need to bring in @jekyll/ecosystem and @jekyll/core so they can comment on your suggestions.

@ghost
Copy link

ghost commented Sep 21, 2016

But you're polling in the test, not using the actual Queue!

@awood
Copy link
Contributor Author

awood commented Dec 6, 2017

@DirtyF Conflicts resolved

@parkr
Copy link
Member

parkr commented Dec 6, 2017

Can you skip the specs that use LiveReload on Windows if they won't work?

@DirtyF DirtyF added the accepted label Dec 6, 2017
@DirtyF DirtyF added this to the v3.7.0 milestone Dec 6, 2017
* Improve variable names
* Improve resiliency when conflicting options are given
* Refactor the LiveReloadReactor class into a separate file
* Correct typos
* Use alias_method for brevity
* Use __dir__ instead of File.dirname(__FILE__)
* Set and use default LiveReload port
@awood
Copy link
Contributor Author

awood commented Dec 6, 2017

Can you skip the specs that use LiveReload on Windows if they won't work?

Done!

@DirtyF
Copy link
Member

DirtyF commented Dec 6, 2017

💯 A big thanks @awood for all the work you put into this, I hope this really improves the developer experience for a majority of Jekyll users.

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 9d68b1b into jekyll:master Dec 6, 2017
jekyllbot added a commit that referenced this pull request Dec 6, 2017
@DirtyF
Copy link
Member

DirtyF commented Dec 6, 2017

@awood

If I add the --livereload option on the command line this works fine. (I only tested this before merging)

But if I add the "livereload" => "true" option to our rake file then the following message appears:

rake aborted!
NoMethodError: undefined method `start' for nil:NilClass
/Users/frank/code/jekyll/jekyll/lib/jekyll/commands/serve.rb:224:in `start_up_webrick'
/Users/frank/code/jekyll/jekyll/lib/jekyll/commands/serve.rb:104:in `process'
/Users/frank/code/jekyll/jekyll/rake/site.rake:37:in `block (2 levels) in <top (required)>'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/task.rb:251:in `block in execute'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/task.rb:251:in `each'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/task.rb:251:in `execute'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/task.rb:195:in `block in invoke_with_call_chain'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/2.4.0/monitor.rb:214:in `mon_synchronize'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/task.rb:188:in `invoke_with_call_chain'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/task.rb:181:in `invoke'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/application.rb:160:in `invoke_task'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/application.rb:116:in `block (2 levels) in top_level'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/application.rb:116:in `each'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/application.rb:116:in `block in top_level'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/application.rb:125:in `run_with_threads'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/application.rb:110:in `top_level'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/application.rb:83:in `block in run'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/application.rb:186:in `standard_exception_handling'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/lib/rake/application.rb:80:in `run'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/gems/2.4.0/gems/rake-12.3.0/exe/rake:27:in `<top (required)>'
/Users/frank/.rbenv/versions/2.4.2/bin/rake:23:in `load'
/Users/frank/.rbenv/versions/2.4.2/bin/rake:23:in `<top (required)>'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/cli/exec.rb:75:in `load'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/cli/exec.rb:75:in `kernel_load'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/cli/exec.rb:28:in `run'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/cli.rb:424:in `exec'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/vendor/thor/lib/thor/invocation.rb:126:in `invoke_command'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/vendor/thor/lib/thor.rb:387:in `dispatch'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/cli.rb:27:in `dispatch'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/vendor/thor/lib/thor/base.rb:466:in `start'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/cli.rb:18:in `start'
/Users/frank/.rbenv/versions/2.4.2/bin/bundle:30:in `block in <main>'
/Users/frank/.rbenv/versions/2.4.2/lib/ruby/site_ruby/2.4.0/bundler/friendly_errors.rb:122:in `with_friendly_errors'
/Users/frank/.rbenv/versions/2.4.2/bin/bundle:22:in `<main>'
Tasks: TOP => site:preview

DirtyF pushed a commit that referenced this pull request Dec 7, 2017
DirtyF pushed a commit that referenced this pull request Dec 7, 2017
@burrich
Copy link

burrich commented Jan 24, 2018

Hi, can you confirm that livereload is not supported on Windows please ?

@awood

@ashmaroli
Copy link
Member

@burrich --livereload is definitely supported on Windows.. just not with --ssl_cert and --ssl_key as well..

@burrich
Copy link

burrich commented Jan 24, 2018

Ok thanks @ashmaroli , i will try.

@awood
Copy link
Contributor Author

awood commented Jan 24, 2018

@burrich If I recall correctly, it won't work with Ruby 2.4 on Windows due to a dependency (EventMachine) not working.

@ashmaroli
Copy link
Member

won't work with Ruby 2.4 on Windows due to a dependency (EventMachine) not working.

@burrich That issue is being tracked here 👉 eventmachine/eventmachine#806

# EventMachine's SSL support in Windows requires building the gem's
# native extensions against OpenSSL and that proved to be a process
# so tedious that expecting users to do it is a non-starter.
Jekyll.logger.abort_with "Error:", "LiveReload does not support SSL"
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've stepped way back from this PR, never had the time to look at it as my plugin was just working for me. However, Livereload can support SSL, I have support for it in my plugin RobertDeRose/jekyll-livereload@34df507

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @RobertDeRose, I was using your plugin until now :)
Are you interested in submitting a patch?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might, if I can find the time. So, that's a big maybe

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RobertDeRose The problem wasn't with LiveReload not supporting SSL; it was with getting EventMachine and OpenSSL to cooperated on Windows. I actually had it working fine in Linux, but getting the OpenSSL libraries to install and work on the Windows CI environment was a nightmare, so I just gave up 😭. I figured most people would only be using LiveReload for rapid prototyping so I didn't consider SSL support to be a must-have.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@awood that's a fair point, I never did testing for Windows, and to be honest, not a lot of benefit for it when developing locally, unless you really need to ensure SSL isn't messing with thing, but at that point, you don't really need live reload. OK ignore my extremely late to the party comments then.

@ghost
Copy link

ghost commented Aug 3, 2018

Fixed ?

@jekyll jekyll locked as resolved and limited conversation to collaborators Aug 3, 2018
@jekyll jekyll deleted a comment Aug 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet